Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add extendable controller & configuration for setup #28

Merged
merged 2 commits into from
Feb 25, 2024
Merged

Add extendable controller & configuration for setup #28

merged 2 commits into from
Feb 25, 2024

Conversation

KiKoS0
Copy link
Collaborator

@KiKoS0 KiKoS0 commented Feb 25, 2024 •
edited
Loading

I think someone with better spring boot knowledge can shape
a better interface to create the Inngest client + CommHandler but
this is already better than manually creating a singleton and
having everything in a random controller.

Note: It might be better to not have a second adapter package and
instead have these spring boot helpers as a feature variant. But
I'll see how it goes with this PR first: #27

I'm also adding a way to build the Inngest client configuration
possibly through a Builder pattern targeted to the specific
environment that the user wants to use.

The current interface (which needs better code documentation) is that a consumer application would subclass the controller and configuration as follows:

ExampleController.java
@RestController
@RequestMapping(value = "/api/inngest")
public class ExampleController extends InngestController {
}
ExampleConfiguration.java
@Configuration
public class ExampleConfiguration extends InngestConfiguration {

    @Override
    public HashMap<String, InngestFunction> functions() {
        HashMap<String, InngestFunction> functions = new HashMap<>();
        ...
        return functions;
    }

    @Override
    public Inngest inngestClient() {
        return new Inngest("spring_example");
    }
}

@KiKoS0 KiKoS0 self-assigned this Feb 25, 2024
@KiKoS0 KiKoS0 force-pushed the extendable-controller branch 2 times, most recently from 8c83b9d to 7d37fac Compare February 25, 2024 09:19
@KiKoS0 KiKoS0 changed the title Add extendable controller & configuration to configure inngest Add extendable controller & configuration for setup Feb 25, 2024
@KiKoS0 KiKoS0 force-pushed the extendable-controller branch from 7d37fac to 9e3bd3c Compare February 25, 2024 09:36
@KiKoS0 KiKoS0 marked this pull request as ready for review February 25, 2024 09:36
@KiKoS0 KiKoS0 force-pushed the extendable-controller branch 2 times, most recently from fec731e to 8e0df9d Compare February 25, 2024 09:39
darwin67
darwin67 approved these changes Feb 25, 2024
View reviewed changes
Copy link
Contributor

@darwin67 darwin67 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine for now.
The idea of #27 is that there will be a serve function that will handle all the routes, and also take the Inngest client as an input so the route handler will have all the information it needs.

Having some compilation error locally, so haven't push the latest code up yet.

@darwin67
Copy link
Contributor

this is the latest code, and kotlin is not happy about something, but can't tell what it is.
https://github.com/inngest/inngest-kt/pull/27/files#diff-a53270cbe78c3842c55b0a20286bd726f41e9e697070455fb5f75c0fb73a4104R15

@KiKoS0
Copy link
Collaborator Author

KiKoS0 commented Feb 25, 2024

oh it was the Kotlin paratrooped it. 😏

@KiKoS0
Remove the singleton in favor of bean injecting the inngest config
875fbef 
I think someone with better spring boot knowledge can shape
a better interface to create the inngest client + CommHandler but
this is already better than manually creating a singleton and
having everything in a random controller.

Note: It might be better to not have a second adapter package and
instead have these spring boot helpers as a feature variant. But
I'll see how it goes with this PR first:

#27

I'm also adding a way to build the Inngest client configuration
possibly through a Builder pattern targeted to the specific
environment that the user wants to use.
@KiKoS0 KiKoS0 force-pushed the extendable-controller branch from 8e0df9d to 875fbef Compare February 25, 2024 09:55
@darwin67
Copy link
Contributor

yeah, I just figured it out. annoying

@KiKoS0 KiKoS0 merged commit 1ac65fc into main Feb 25, 2024
7 checks passed
@KiKoS0 KiKoS0 deleted the extendable-controller branch February 25, 2024 10:00
albertchae
albertchae reviewed Feb 25, 2024
View reviewed changes
inngest-spring-boot-adapter/build.gradle.kts
}

group = "com.inngest"
version = "0.0.1-SNAPSHOT"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably best to version all the subpackages of inngest-kt together. I think there's something we can add in the root gradle file. I'll take a note of this for now

inngest-spring-boot-demo/src/main/java/com/inngest/springbootdemo/DemoConfiguration.java

import com.inngest.*;
import com.inngest.springboot.InngestConfiguration;
import kotlin.jvm.functions.Function2;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already discussed this but probably ideal if we don't have to pull in a kotlin Function here and can instead use lambdas (available in Java 8) or something like https://docs.oracle.com/javase/8/docs/api/java/lang/Runnable.html

@KiKoS0 KiKoS0 mentioned this pull request Feb 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@albertchae albertchae albertchae left review comments

@darwin67 darwin67 darwin67 approved these changes

@djfarrelly djfarrelly Awaiting requested review from djfarrelly djfarrelly is a code owner

@jpwilliams jpwilliams Awaiting requested review from jpwilliams jpwilliams is a code owner

@tonyhb tonyhb Awaiting requested review from tonyhb tonyhb is a code owner

Assignees

@KiKoS0 KiKoS0

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@KiKoS0 @darwin67 @albertchae